-
Notifications
You must be signed in to change notification settings - Fork 232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IPIP-328: JSON and CBOR Response Formats on HTTP Gateways #328
Conversation
hacdias
commented
Oct 7, 2022
•
edited
Loading
edited
- Text fixtures in the specs and pinned
- Working code: feat(gateway): JSON and CBOR response formats (IPIP-328) kubo#9335
- Wait until we have new media types registered at IANA and reference them in the specs – tracked in:
- [IANA #1240938] application/vnd.ipld.dag-cbor registration request in-web-browsers#201
- [IANA #1240939] application/vnd.ipld.dag-json registration request in-web-browsers#202
0a081f4
to
4134864
Compare
@lidel I reached a nice first-draft. There's some sections that are yet incomplete, but I'm marking this as ready for review as it is definitely ready for receiving some feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hacdias thank you! Did a quick pass with early feedback which should help you with next steps here.
|
||
## Test fixtures | ||
|
||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want at minimum to provide basic test vectors (we want to have them in Kubo tests and only reference CIDs here)
- CID with
dag-pb
codec and a corresponding Logical DAG-JSON representation that is expected to be returned when?format=dag-json
is passed - CID with
json
codec that is a valid JSON but not a valid DAG-JSON - CID with
cbor
codec that is a valid CBOR but not a valid DAG-CBOR - CID with
dag-json
codec- dag-json document should link to other dag-json, so we can test DAG-traversal
/ipfs/{dag-json-cid}/field-linking-to-other-dag-json/
– this should be enough smoke-test
- dag-json document should link to other dag-json, so we can test DAG-traversal
- CID with
dag-cbor
codec- dag-cbor document should link to other dag-cbor, so we can test DAG-traversal
/ipfs/{dag-cbor-cid}/field-linking-to-other-dag-cbor/
– this should be enough smoke-test
- dag-cbor document should link to other dag-cbor, so we can test DAG-traversal
+ invalid versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidel Unfortunately, I'm not sure if this will be easy to do. See ipfs/kubo#9335 (comment). It seems that the Go codecs encoders are quite strict, making the differences between both codecs very minimal. I thought that we could play with the same JSON document containing a link: in JSON it won't be resolved, in DAG-JSON it will. Same for CBOR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test fixtures are also welcome at https://github.com/ipld/codec-fixtures :) We especially are missing failing ones. So if you create some, perhaps also add them there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vmx @lidel regarding invalid versions, I'm thinking about what we could do. We would need something that is valid in some format, such as CBOR, but not valid in other format, such as JSON. The problem being that we would need to add the data to IPFS somehow and the serialisation process would already check if the data is valid or not for the codec.
Considering that DAG-JSON and DAG-CBOR are both subsets of their non DAG-* versions, I can't think of anything on top of my mind that would work. We could see if we could add a weird CBOR that would fail when requested via format=json
/format=dag-json
(as both are equivalent when requesting a CBOR document.
I think this is related to what I mentioned on the previous comment: that the Go impl. of CBOR and JSON are quite strict by themselves, so adding something to IPFS that would then fail when downloading through the gateway would be quite hard. Am I missing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit I'm not deep into this whole IPIP, I just read "fixtures" and "DAG-CBOR/JSON" and jumped in :)
Valid JSON, but invalid DAG-JSON is anything with strange things after a property with just a slash, e.g. {"/": true}
. Valid CBOR, but invalid DAG-CBOR: tags that are not tag 42. But of course all valid DAG-CBOR/JSON should be valid in the non DAG*-versions. Also valid DAG-JSON and DAG-CBOR should be interchangeable with each other.
Co-authored-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
e4cde04
to
a0c86bb
Compare
@lidel I have addressed most of your feedback and I think the IPIP is quite complete. I'm going to work on figuring out if I can find any JSON/CBOR that is invalid DAG-JSOn and DAG-CBOR. As I mentioned in the comment, I think that might be a bit difficult, specially since some of the constraints of DAG-JSON are also present in Go's JSON implementation (large numbers for example). |
#9335 ipfs/specs#328 Co-authored-by: Marcin Rataj <lidel@lidel.org>
We've discussed this during IPFS-Implementers-Sync-2022-12-08 and no concerns were raised, @b5 (Iroh) +1'd, decision was made to ratify this. I'll merge this PR when a stable Kubo 0.18 ships with the implementation. |
3e5fbc7
to
e4cbfd1
Compare
@lidel I think the IPIP explains the current behaviour well and can be safely merged as Kubo 0.18 has been shipped with this functionality. |
ipfs/kubo#9335 ipfs/specs#328 Co-authored-by: Marcin Rataj <lidel@lidel.org> This commit was moved from ipfs/kubo@fdd1965
My understanding is the remaining step is for @lidel to give a last read through and merge. |
Thank to @hacdias and everyone involved in this over the years. For drive-by-reader: reference implementation of this IPIP shipped in Kubo 0.18 |